feat: expose access group support in DA #314

alex-reiff · 2025-03-26T19:57:42Z

Description

Release required?

No release
Patch release (x.x.X)
Minor release (x.X.x)
Major release (X.x.x)

Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

If relevant, a test for the change is included or updated with this PR.
If relevant, documentation for the change is included or updated with this PR.

For mergers

Use a conventional commit message to set the release level. Follow the guidelines.
Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
Use the Squash and merge option.

alex-reiff · 2025-03-26T19:57:53Z

/run pipeline

alex-reiff · 2025-03-26T20:28:23Z

         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | Warning: Value for undeclared variable
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | The root module does not declare a variable named "private_engine_enabled"
         2025/03/26 20:18:10 Terraform apply | but a value was found in file "schematics.tfvars". If you meant to use this
         2025/03/26 20:18:10 Terraform apply | value, add a "variable" block to the configuration.
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | To silence these warnings, use TF_VAR_... environment variables to provide
         2025/03/26 20:18:10 Terraform apply | certain "global" settings to all configurations in your organization. To
         2025/03/26 20:18:10 Terraform apply | reduce the verbosity of these warnings, use the -compact-warnings option.
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | Warning: Value for undeclared variable
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | The root module does not declare a variable named
         2025/03/26 20:18:10 Terraform apply | "existing_event_notification_instance_crn" but a value was found in file
         2025/03/26 20:18:10 Terraform apply | "schematics.tfvars". If you meant to use this value, add a "variable" block
         2025/03/26 20:18:10 Terraform apply | to the configuration.
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | To silence these warnings, use TF_VAR_... environment variables to provide
         2025/03/26 20:18:10 Terraform apply | certain "global" settings to all configurations in your organization. To
         2025/03/26 20:18:10 Terraform apply | reduce the verbosity of these warnings, use the -compact-warnings option.
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | Warning: Values for undeclared variables
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | In addition to the other similar warnings shown, 1 other variable(s) defined
         2025/03/26 20:18:10 Terraform apply | without being declared.
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | Error: ---
         2025/03/26 20:18:10 Terraform apply | id: terraform-6039814c
         2025/03/26 20:18:10 Terraform apply | summary: |
         2025/03/26 20:18:10 Terraform apply |   CreateSecretGroupWithContext failed: Post "https://6daad330-cd07-4e78-bf64-1de86691e395.eu-de.secrets-manager.appdomain.cloud/api/v2/secret_groups": dial tcp: lookup 6daad330-cd07-4e78-bf64-1de86691e395.eu-de.secrets-manager.appdomain.cloud on 172.21.0.10:53: no such host
         2025/03/26 20:18:10 Terraform apply |   null
         2025/03/26 20:18:10 Terraform apply | severity: error
         2025/03/26 20:18:10 Terraform apply | resource: ibm_sm_secret_group
         2025/03/26 20:18:10 Terraform apply | operation: create
         2025/03/26 20:18:10 Terraform apply | component:
         2025/03/26 20:18:10 Terraform apply |   name: github.com/IBM-Cloud/terraform-provider-ibm
         2025/03/26 20:18:10 Terraform apply |   version: 1.76.2
         2025/03/26 20:18:10 Terraform apply | ---
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform apply |   with module.secrets_manager_group_acct.ibm_sm_secret_group.secret_group,
         2025/03/26 20:18:10 Terraform apply |   on .terraform/modules/secrets_manager_group_acct/main.tf line 8, in resource "ibm_sm_secret_group" "secret_group":
         2025/03/26 20:18:10 Terraform apply |    8: resource "ibm_sm_secret_group" "secret_group" {
         2025/03/26 20:18:10 Terraform apply | 
         2025/03/26 20:18:10 Terraform APPLY error: Terraform APPLY errorexit status 1
         2025/03/26 20:18:10 Could not execute job: Error : Terraform APPLY errorexit status 1

alex-reiff · 2025-03-27T04:02:34Z

/run pipeline

alex-reiff · 2025-03-28T02:26:35Z

same error occurs on main branch, still can't figure out what changed.

alex-reiff · 2025-03-28T17:21:32Z

/run pipeline

…nforced"<br>- The "standard" variation has been deprecated does not exist in this release (#300) BREAKING CHANGE: There is no upgrade path from the deprecated "Standard" DA variation to either of the new "Fully configurable" or "Security-enforced variations

alex-reiff · 2025-03-28T18:05:05Z

/run pipeline

alex-reiff · 2025-03-28T18:59:14Z

/run pipeline

alex-reiff · 2025-03-30T03:24:55Z

/run pipeline

alex-reiff · 2025-03-31T17:12:37Z

Only test currently failing locally is due to the validation bug introduced in secret-manager-secret-group v1.3. Fix for that here: terraform-ibm-modules/terraform-ibm-secrets-manager-secret-group#277

alex-reiff · 2025-03-31T18:04:47Z

/run pipeline

ocofaigh

I think we need to make this more flexible. Expose a "list of objects" type input where user can create multiple secret groups with associated access groups. Perhaps give it a default value with 1 group (General) with SecretsReader access, but it must be exposed.
We would need a markdown helper doc too so users will know the required format, and the variable descrption should link to it.

ocofaigh · 2025-03-31T19:39:39Z

solutions/fully-configurable/main.tf

  secrets_manager_guid                = var.existing_secrets_manager_crn != null ? (length(local.parsed_existing_secrets_manager_crn) > 0 ? local.parsed_existing_secrets_manager_crn[7] : null) : module.secrets_manager.secrets_manager_guid
  secrets_manager_crn                 = var.existing_secrets_manager_crn != null ? var.existing_secrets_manager_crn : module.secrets_manager.secrets_manager_crn
  secrets_manager_region              = var.existing_secrets_manager_crn != null ? (length(local.parsed_existing_secrets_manager_crn) > 0 ? local.parsed_existing_secrets_manager_crn[5] : null) : module.secrets_manager.secrets_manager_region
+  secrets_manager_endpoint_type       = var.existing_secrets_manager_crn != null ? (length(local.parsed_existing_secrets_manager_crn) > 0 ? local.parsed_existing_secrets_manager_crn[3] : null) : var.secrets_manager_endpoint_type


There is already a variable for this var.secrets_manager_endpoint_type

ocofaigh · 2025-03-31T19:41:47Z

solutions/fully-configurable/main.tf

 }

+module "secrets_manager_group" {
+  depends_on           = [module.secrets_manager]


why the explicit depends_on? Its a bad practise, there is already an implicit dependency by passing the guid to this module

ocofaigh · 2025-03-31T19:46:50Z

solutions/fully-configurable/main.tf

+  secret_group_name        = "General"                        #checkov:skip=CKV_SECRET_6: does not require high entropy string as is static value
+  secret_group_description = "Initially created secret group" #tfsec:ignore:general-secrets-no-plaintext-exposure
+  create_access_group      = var.create_general_secret_group_access_group
+  access_group_name        = "${var.prefix}-General-secrets-group-access-group"


prefix is optional (it can be null). Ensure to account for this like we do everywhere else prefix is used

alex-reiff · 2025-03-31T20:06:53Z

@ocofaigh Switched it to the secrets object as an input.

alex-reiff · 2025-03-31T20:14:05Z

/run pipeline

ocofaigh

The DA should only support creating secret group / access group. Not secrets too

ocofaigh

see latest comments

ocofaigh · 2025-03-31T20:39:09Z

solutions/fully-configurable/variables.tf

+      secret_group_name   = "General"
+      create_access_group = true
+      access_group_name   = "general-secrets-group-access-group"
+      access_group_roles  = ["SecretsReader"]


Can we add secret_group_description value

ocofaigh · 2025-03-31T20:40:45Z

solutions/fully-configurable/variables.tf

+    {
+      secret_group_name   = "General"
+      create_access_group = true
+      access_group_name   = "general-secrets-group-access-group"


hmm, this is going to cause us issues in our account since access group names have to be unique. Perhaps we override it in our tests?

ocofaigh · 2025-03-31T20:42:13Z

solutions/fully-configurable/variables.tf

+    access_group_roles       = optional(list(string))
+    access_group_tags        = optional(list(string))
+  }))
+  description = "Secret Manager secret group configurations."


Description should mention about access group support too. And it should link to a helper markdown doc

ocofaigh · 2025-03-31T20:42:28Z

solutions/fully-configurable/variables.tf

+    secret_group_name        = string
+    secret_group_description = optional(string)
+    existing_secret_group    = optional(bool, false)
+    create_access_group      = optional(bool, false)


I would default this to true as a best practise

if we default to true, have to set a default for access_group_roles

ocofaigh · 2025-03-31T20:42:45Z

solutions/fully-configurable/variables.tf

+  type = list(object({
+    secret_group_name        = string
+    secret_group_description = optional(string)
+    existing_secret_group    = optional(bool, false)


not sure we need to support existing_secret_group

alex-reiff · 2025-03-31T23:05:57Z

/run pipeline

ocofaigh

You forgot to expose it in the security enforced variation
The diagram should be updated to show the secret group and access group support and it should be included as features in the ibm_catalog.json

ocofaigh · 2025-04-01T08:30:05Z

solutions/fully-configurable/variables.tf

  }
 }

+variable "secrets" {


This should be called secret_groups

ocofaigh · 2025-04-01T08:31:24Z

solutions/fully-configurable/variables.tf

+  default = [
+    {
+      secret_group_name        = "General"
+      secret_group_description = "A general purpose secrets group with an associated access group"


Suggested change

secret_group_description = "A general purpose secrets group with an associated access group"

secret_group_description = "A general purpose secrets group with an associated access group which has a secrets reader role"

ocofaigh · 2025-04-01T08:35:09Z

solutions/fully-configurable/variables.tf

+      true if(group.create_access_group && group.access_group_roles == null)
+    ]) == 0
+  }
+}


I think this has to be set to nullable = false since there is a for loop used here. You should add it to the secrets input in both the root module and submodule too.

@ocofaigh Is it possible to set one field inside the variable as not nullable?

no, but the whole variable should be set to nullable = false because the for loop would fail if it was null. Use would just pass [] if they didnt wan't to create any groups

ocofaigh · 2025-04-01T08:38:22Z

tests/pr_test.go

 		{Name: "region", Value: validRegions[rand.Intn(len(validRegions))], DataType: "string"},
 		{Name: "existing_resource_group_name", Value: resourceGroup, DataType: "string"},
 		{Name: "service_plan", Value: "trial", DataType: "string"},
+		{Name: "secrets", Value: []map[string]interface{}{


you have duplicated this code in every test. Suggest to store it as a global variable and re-use it instead of duplicating it

ocofaigh

The diagram is not displaying well in dark mode

ocofaigh · 2025-04-01T14:47:24Z

/run pipeline

terraform-ibm-modules-ops · 2025-04-01T15:24:43Z

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

GitHub release
v2.1.0

Your semantic-release bot 📦🚀

expose access group support in DA

8b3eff2

alex-reiff requested a review from shemau as a code owner March 26, 2025 19:57

Aayush-Abhyarthi and others added 3 commits March 28, 2025 14:03

update rebase

c6446a8

Merge branch 'main' into access-group2

f7a8799

pass thru endpoints

0fd93ab

minor tweaks

42e665f

alex-reiff added 2 commits March 31, 2025 13:21

parameterize

3675d8d

bump secret group

641648d

alex-reiff requested review from ocofaigh and toddgiguere March 31, 2025 19:29

ocofaigh requested changes Mar 31, 2025

View reviewed changes

customizable input

edb8fc4

alex-reiff requested a review from ocofaigh March 31, 2025 20:06

ocofaigh requested changes Mar 31, 2025

View reviewed changes

alex-reiff added 2 commits March 31, 2025 16:28

dont expose secrets + parallel tests

21beb0e

dont expose secrets + parallel tests

c303ae5

alex-reiff requested a review from ocofaigh March 31, 2025 20:34

ocofaigh requested changes Mar 31, 2025

View reviewed changes

alex-reiff added 2 commits March 31, 2025 17:13

extended doc for secret group input

2a0596b

unique names in test

40c346d

minor test tweak

ec18731

ocofaigh requested changes Apr 1, 2025

View reviewed changes

alex-reiff added 3 commits April 1, 2025 09:49

catalog updates + test tweaks

5042751

fix svg

424c45e

doc tweak

3046c6c

alex-reiff requested a review from ocofaigh April 1, 2025 14:08

ocofaigh requested changes Apr 1, 2025

View reviewed changes

alex-reiff added 2 commits April 1, 2025 10:34

dark mode diagram

fee85f7

not nullable

f8df202

ocofaigh approved these changes Apr 1, 2025

View reviewed changes

ocofaigh merged commit 600945c into main Apr 1, 2025
2 checks passed

ocofaigh deleted the access-group2 branch April 1, 2025 15:23

terraform-ibm-modules-ops added the released label Apr 1, 2025

	secret_group_description = "A general purpose secrets group with an associated access group"
	secret_group_description = "A general purpose secrets group with an associated access group which has a secrets reader role"

feat: expose access group support in DA #314

feat: expose access group support in DA #314

Uh oh!

Conversation

alex-reiff commented Mar 26, 2025

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

alex-reiff commented Mar 26, 2025

Uh oh!

alex-reiff commented Mar 26, 2025

Uh oh!

alex-reiff commented Mar 27, 2025

Uh oh!

alex-reiff commented Mar 28, 2025

Uh oh!

alex-reiff commented Mar 28, 2025

Uh oh!

alex-reiff commented Mar 28, 2025

Uh oh!

alex-reiff commented Mar 28, 2025

Uh oh!

alex-reiff commented Mar 30, 2025

Uh oh!

alex-reiff commented Mar 31, 2025

Uh oh!

alex-reiff commented Mar 31, 2025

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-reiff commented Mar 31, 2025

Uh oh!

alex-reiff commented Mar 31, 2025

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

ocofaigh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-reiff commented Mar 31, 2025

Uh oh!

ocofaigh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ocofaigh left a comment •

edited

Loading